[codex] Fix narrow passband drag hit testing#3523
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes hit-testing for very narrow rendered RX filter passbands in SpectrumWidget so dragging inside the passband reliably moves the slice (VFO drag) instead of accidentally initiating a filter-edge resize when zoomed out.
Changes:
- Added shared pixel-based passband hit-test helpers to classify edge vs body interactions for narrow passbands.
- Updated active-slice
mousePressEvent()logic to use the new edge/body classifier. - Updated hover cursor edge detection in
mouseMoveEvent()to use the same classifier.
24ea85c to
d0f9b48
Compare
There was a problem hiding this comment.
Thanks @rfoust — this is a well-structured fix. Centralizing the edge/body classification so press, hover, the VFO-child event filter, and the QRhi polling path all agree is the right shape, and I verified the supporting details: setSpectrumCursor() already guards redundant installs (so the per-frame call in renderGpuFrame is cheap), VfoWidget builds all its children in buildUI() at construction (so installVfoCursorEventFilter catches them all), and the event filter never consumes events, so child buttons keep their clicks as described.
One real inconsistency — hover/press ordering for overlapping slices
sliceCursorShapeAt() checks the active overlay first, then inactive slices. mousePressEvent() does the opposite: the inactive-slice loop (badge → centerline → passband body, all emitting sliceClicked) runs before the active overlay's edge/body checks. So when an inactive slice's badge or passband overlaps the active slice's edge zone or passband — exactly the zoomed-out, slices-close scenario this PR targets — the cursor shows SizeHor/OpenHand (active-slice action) but the click activates the inactive slice instead. Since the whole point of the cursor work is previewing the action, suggest reordering sliceCursorShapeAt() to check inactive slices first, mirroring press order.
On the Copilot findings
- "
filterInteriorGrabPxforcesinsideGrabPx >= 1, leaving a 7px passband only 5px of body" — mostly a false positive. There is no>= 1clamp; forwidthPx = 7,(7 - 6) / 2 == 0, so no interior pixels are stolen and the body is the full 6px. However, there is a genuine off-by-one for even widths: the edge zones include their boundary pixel (mx <= loX + insideGrabPx), so the body iswidthPx − 2·insideGrabPx − 1pixels — 5px instead of 6 for widths 8, 10, …, 22. If the ≥6px guarantee is meant to be strict,(widthPx - kFilterPassbandMinBodyPx - 1) / 2fixes it. Minor either way. - "Hover still uses a 5px grab radius vs 8px on press" — false positive. This PR deletes the old
GRAB = 5hover path; hover now goes throughsliceCursorShapeAt()→filterEdgeHitAtPixel()with the samekFilterEdgeGrabPx = 8as press.
Two small notes
- The
childAt()guard bypass inmouseMoveEventmeans the #2355 tooltip protection no longer applies where overlay-menu buttons overlap passband/edge zones —QToolTip::hideText()paths can kill their tooltips again there. Probably an acceptable tradeoff for the cursor affordance, but worth being aware it partially reverts #2355 in those zones. - The validation section says "Manual reporter retest confirmed the cursor behavior was better before this push" — as written that says the reporter preferred the previous behavior. Assuming that's a typo for "better than before this push", but please confirm.
No scope, settings-convention, or resource concerns — the diff matches the stated scope and the new helpers are pure functions.
🤖 aethersdr-agent · cost: $10.5651 · model: claude-fable-5
d0f9b48 to
697b28b
Compare
|
Updated after the latest review and upstream rebase. Addressed review feedback:
Notes on prior comments:
Validation on the rebased head
|
cb72405 to
b1f7cc5
Compare
|
Pushed follow-up cursor fixes in Root cause for this round: inactive slice hover did not run the shared Changes in this update:
Validation:
|
b1f7cc5 to
9b5744b
Compare
NF0T
left a comment
There was a problem hiding this comment.
Claimed; read full diff, commit, PR body, both bot reviews, and all comments including today's inactive-slice follow-up.
Root causes verified real — all three. The edge grab zone overlap on narrow passbands is a direct consequence of std::abs(mx - loX) <= 8 with no passband-width awareness: on a 4px rendered filter both zones cover the entire passband. The child-widget cursor bypass and the inactive-slice ordering mismatch are independently reproduced by the QRhi polling path and the pre-edge badge check respectively — the three bugs are layered but structurally distinct.
Shared hit-test helpers are correct:
filterInteriorGrabPx — (widthPx - kFilterPassbandMinBodyPx - 1) / 2 with the -1 ensuring integer division rounds down, so the minimum body area is preserved at every boundary width. Returns 0 when widthPx <= kFilterPassbandMinBodyPx, collapsing both edge zones entirely on a sub-6px passband rather than producing a negative interior.
filterEdgeHitAtPixel — handles loX > hiX (LSB filter orientation) correctly via the lowIsLeft flag. The both-edges-in-range disambiguation by proximity to each edge is the same tie-breaking logic that was already in the old active-slice path (#764), now unified.
kFilterEdgeGrabPx = 8 used consistently across press and hover. The old code used GRAB = 5 in the hover path and GRAB = 8 in press — that mismatch meant the hover cursor preview didn't reflect the actual click zone. Consistent 8px is correct.
Cursor override mechanism is correct. Three QVariant properties save/restore each VFO child's prior cursor state without adding member variables. clearCursorOverride correctly distinguishes WA_SetCursor (restore) vs inheriting (unsetCursor). The active guard prevents double-saving on repeated calls before a clear. installVfoCursorEventFilter is called from addVfoWidget() so all children are covered at construction. eventFilter maps child coordinates to spectrum-local via mapFromGlobal() — correct for overlaid widgets.
sliceCursorShapeAt() ordering. Inactive slices checked before active — matches mousePressEvent priority after today's rebase, fixing Bug 3. The negative early-return on out-of-spectrum coordinates prevents false matches.
mouseMoveEvent child-widget guard. Allowing through when overSliceCursorTarget is true preserves VFO child tooltip behavior (#2355) while ensuring the cursor advertises the drag action even when a child button is underneath.
macOS normalization extracted to normalizedSpectrumCursorShape() and shared between setSpectrumCursor() and setCursorOverride() — eliminates the prior risk of the two paths diverging.
Scope discipline. SpectrumWidget.{cpp,h} only. Zero #3351 exposure. No conflict with #3547 (which modifies sliceColorForOverlay() — a different code region entirely).
CI fully green (all 6 checks including CodeQL, completed 14:20 UTC today). Already rebased onto upstream/main.
Approved.
Root cause
SpectrumWidget::mousePressEvent()used a fixed 8 px grab zone for each filter edge before checking whether the click was inside the passband body. When the panadapter was zoomed far enough out, a narrow slice passband could render at only a few pixels wide, so the edge grab zones overlapped most or all of the passband. A click intended to drag the whole slice was then classified as a filter-edge drag and changed the passband width instead.A second cursor-specific issue hid the intended affordance after the hit testing was corrected: the hover cursor path could be bypassed when VFO child widgets covered the passband area, and the QRhi render-time cursor polling path updated tracking overlays without also updating the OS cursor shape. That left users seeing the inherited crosshair, or a pointing-hand cursor from an overlaid VFO child button, instead of the resize cursor at the true filter edges.
The latest follow-up found one more inactive-slice-specific mismatch: inactive slice hover checked the slice flag/centerline activation target before it checked the passband edge classifier. An inactive edge near the slice flag could therefore keep the crosshair or activation cursor even though the active-slice edge path showed the resize cursor correctly.
User impact
Users with narrow RX filters on a zoomed-out panadapter can move a slice by dragging inside the passband instead of accidentally latching onto an edge. Edge resizing remains available near the passband edges.
The hover cursor now previews the action before clicking: horizontal resize at filter edges, open hand for passband-body moves, closed hand once the move drag is active, and existing pointing-hand behavior for actual slice/button activation targets. This now applies consistently to active and inactive slice edges, including edges adjacent to the slice flag.
Change summary
freqScaleH()layout math in the cursor helper and conflict resolution.upstream/main(a96b0bc9).Validation
git diff --check HEAD~1 HEADpython3 tools/check_a11y.py src/gui/SpectrumWidget.cpp src/gui/SpectrumWidget.hcmake --build build -j8on head9b5744b3ctest --test-dir build --output-on-failure --parallel 8 -E theme_manager_testpassed 32/32 tests